wolfsshd: make the supplementary-group drop reliable and tested#1085
Open
yosuke-wolfssl wants to merge 1 commit into
Open
wolfsshd: make the supplementary-group drop reliable and tested#1085yosuke-wolfssl wants to merge 1 commit into
yosuke-wolfssl wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds unit-test indirection hooks around getgrouplist() and setgroups() in wolfSSHD_AuthSetGroups, plus new unit tests to ensure the supplementary-group drop is executed and failures are handled, improving mutation coverage and reducing risk of privilege retention regressions.
Changes:
- Add
WOLFSSHD_UNIT_TESTcallback hooks forsetgroups()andgetgrouplist()and routewolfSSHD_AuthSetGroupsthrough them when unit-testing. - Add deterministic unit tests for the supplementary-group drop success path and for
getgrouplist/setgroupsfailure handling. - Standardize Windows guards in the touched area to
_WIN32for consistency with existing conventions in these files.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/wolfsshd/auth.c | Adds default adapters and unit-test callback routing for getgrouplist/setgroups inside wolfSSHD_AuthSetGroups. |
| apps/wolfsshd/auth.h | Exposes new unit-test callback externs for setgroups/getgrouplist under WOLFSSHD_UNIT_TEST on non-Windows builds. |
| apps/wolfsshd/test/test_configuration.c | Introduces group-resolution and setgroups stubs plus three new unit tests covering the supplementary-group drop behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
72efb4a to
da908a0
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1085
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wolfSSHD_AuthSetGroupsperforms the daemon's supplementary-group drop — it resolves the target user's full group list and applies it withsetgroups()beforewolfSSHD_AuthReducePermissionsUserreduces uid/gid. This PR started as mutation-coverage for that drop (finding f_6701) and, in the process, surfaced and fixes a real privilege-retention bug on macOS/BSD.Motivation
Coverage gap (f_6701). Deleting the
setgroups()call, or defeating its== -1failure guard, survived the entire test suite. Unlike thesetregid/setreuidpath (baseline 5578), there was noWOLFSSHD_UNIT_TESThook forsetgroups, so no test observed whether it ran. With the call gone, the spawned user session retains the wolfsshd (root) process's supplementary group memberships — a concrete privilege-retention path.Bug found while writing the test.
wolfSSHD_AuthSetGroupsgated the wholesetgroups()block on a NULL-sizegetgrouplist()probe returning-1. That probe is unreliable: on macOS/BSD it returns success with size 0, so the block was skipped and the supplementary-group drop silently never ran on those platforms. The file already documents this exact unreliability and solves it with grow-and-retry inwolfSSHD_GetUserGroupNames.Changes
WOLFSSHD_UNIT_TESTcallbackswsshd_setgroups_cbandwsshd_getgrouplist_cb, alongside the existingwsshd_setregid_cb/wsshd_setreuid_cb. Each default forwards to the real syscall through a one-line adapter, becausesetgroups()/getgrouplist()prototypes differ across platforms (size_tvsint,gid_t*vsint*) and a single portable function-pointer type can't bind to them directly.wolfSSHD_GetUserGroupNamesinto a sharedwolfSSHD_GetUserGroupListhelper, and call it from bothGetUserGroupNamesandAuthSetGroups. The list is now resolved through one portable path andsetgroups()runs on every platform._WIN32guard macro (matching theauth.hexterns and the file's majority convention), so callback definitions and their references share one Windows guard and no_WIN32 && !WIN32build can compile a caller without its definition.Tests
The tests mock
getgrouplistandsetgroups, making them deterministic on every platform (the real macOS NULL-size probe would otherwise skip the drop entirely on the dev machine):test_AuthSetGroups_ok— assertssetgroups()is invoked with the resolved group count and the exact forwarded gids, after at least one buffer grow-and-retry (the test build forces-DWOLFSSHD_GROUP_LIST_INIT=1).test_AuthSetGroups_setgroups_fail— asetgroups()failure must propagateWS_FATAL_ERROR.test_AuthSetGroups_getgrouplist_fail— a lookup that never succeeds must fail closed and never reachsetgroups().Verification
Bug demonstrated & fixed (real, unmocked, on macOS):
ret=0 sz=0→ setgroups SKIPPEDMutation kills — each defect is caught by exactly the corresponding test (others still pass):
setgroups()calltest_AuthSetGroups_oksetgroups()== -1guardtest_AuthSetGroups_setgroups_failtest_AuthSetGroups_getgrouplist_failwolfsshdunit suite: 103 passed, 0 failures; normal build clean, no new warnings.-fsanitize=address,undefined; LeakSanitizer disabled on Darwin).Notes
wolfSSHD_GetUserGroupNamesbehavior is unchanged (it already used this exact sizing); it now shares the helper instead of duplicating it.0on success under QNX to match thegetgrouplistcontract).WOLFSSHD_UNIT_TESTbuilds beyond the sizing fix itself.